Add support for subcycling to Thompson MP, remove SIONLIB code#666
Add support for subcycling to Thompson MP, remove SIONLIB code#666climbfuji wants to merge 11 commits into
Conversation
…cies, use assumed-size arrays
climbfuji
changed the title
climbfuji
requested review from
JulieSchramm,
grantfirl and
llpcarson
as code owners
|
For your awareness, EMC is testing the sub-cycling in both RRFS, and GFS (v16 suite) at different resolutions. Please let us know if anyone else is also conducting similar testing and evaluation. Hopefully all tests can be completed before this PR is approved for commit. |
It would be great to have this tested, definitely. The basics of this PR, namely the capability for CCPP to do subcycling of schemes, should be merged in any case (it is a CCPP design specification to be able to do that, and the changes are generic in that sense). If it doesn't help with Thompson in our tests, we simply reset the number of subcycles to 1 in the suite definition file, and everything works as before the PR. |
|
@yangfanglin @climbfuji We presented at the meeting the results from the global retros with syb-cycling for the GSD_noah physics suite. Today I got the AC verification for the sub-cycled run that shows same or slightly improved performance compared to the Control run without sub-cycling. Please, see the plots of AC for Northern and Southern hemisphere (the test is in blue): |
|
Tayan. thank you for sharing the ACC plots. They are definitely
encouraging.
…On Thu, May 27, 2021 at 5:27 PM tanyasmirnova ***@***.***> wrote:
@yangfanglin <https://github.com/yangfanglin> @climbfuji
<https://github.com/climbfuji> We presented at the meeting the results
from the global retros with syb-cycling for the GSD_noah physics suite.
Today I got the AC verification for the sub-cycled run that shows same or
slightly improved performance compared to the Control run without
sub-cycling. Please, see the plots of AC for Northern and Southern
hemisphere (the test is in blue):
[image: AC_nsub3_NH]
<https://user-images.githubusercontent.com/38667904/119899041-d9814e00-beff-11eb-9de9-d492aa5693a3.png>
[image: AC_nsub3_SH]
<https://user-images.githubusercontent.com/38667904/119899079-e56d1000-beff-11eb-9c0f-cc04b5579fb0.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#666 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKY5N2IDATA7GC56XYPSMXTTP22K3ANCNFSM45NPKF7Q>
.
--
*Fanglin Yang, Ph.D.*
*Chief, Model Physics Group*
*Modeling and Data Assimilation Branch*
*NOAA/NWS/NCEP Environmental Modeling Center*
*https://www.emc.ncep.noaa.gov/gmb/wx24fy/fyang/
<https://www.emc.ncep.noaa.gov/gmb/wx24fy/fyang/>*
|
…pson_subcycling_for_main
|
@gthompsnWRF we are planning to merge this PR on June 9/10 as part of a tutorial for merging nested repositories (submodules). Do you think you can review the code changes beforehand? Thanks! |
|
|
||
| ! Recompute sr at last subcycling step | ||
| if (nsteps>1 .and. istep == nsteps) then | ||
| sr = (snow + graupel + ice)/(rain+1.e-12) |
There was a problem hiding this comment.
There is definitely a bug in here. In WRF, the variable called "rainncv" is actually ALL precip, not rain by itself. The calculation of "sr" should be the quotient of frozen precip types divided by all precip (frozen plus rain). So this bug may exist in an earlier code version as well, although it may not matter. In WRF, "sr" was used to tell the land-surface-model what fraction of precip is frozen, but sr might be ignored in SCM or CCPP. Also, what about the condition when nstep==1? Isn't that leaving sr uninitialized? Could the IF-test just be istep==nsteps so that both are 1 or both are not one?
There was a problem hiding this comment.
Good find. The code was copied from inside module_mp_thompson, in which, as you say correctly, rain includes frozen precip. But because of line 675 in the new code, we need to add snow, graupel and ice to the denominator. Will fix in a bit.
There was a problem hiding this comment.
I forgot to answer the nstep==1 question. Since I did not remove the original calculation of sr inside module_mp_thompson, sr gets calculated for every substep inside your code (and correctly, since inside module_mp_thompson rain contains all frozen precip). Thus, the value coming from module_mp_thompson for nsteps==1 is correct and there is no need to recalculate it again. It is only for nsteps>1 that we want to compute sr for the entire precipipation generated by all substeps of Thompson, not just the last substep. Of course I need to fix the bug and add snow, graupel, ice to rain on the r.h.s. of line 679.
| ims=ims, ime=ime, jms=jms, jme=jme, kms=kms, kme=kme, & | ||
| its=its, ite=ite, jts=jts, jte=jte, kts=kts, kte=kte, & | ||
| errmsg=errmsg, errflg=errflg, reset=reset) | ||
| reset=reset, istep=istep, nsteps=nsteps, & |
There was a problem hiding this comment.
I did not see this before, but the variable name called "reset" is utterly confusing. I dig into where it controls something, and it is related to calculating radar reflectivity with or without the melting ice option. The word reset sure seems to imply something deeper to me. In the interest of not introducing more changes, I guess I don't care, but picking variable names with a bit more care would be preferred.
There was a problem hiding this comment.
This code is from @ericaligo-NOAA and was merged some long time ago. Reset is a confusing word, I totally agree. Maybe @ericaligo-NOAA can suggest a better name.
There was a problem hiding this comment.
Perhaps the simplest and more interpretable name is just: reset_dBZ. How does that sound to everyone?
| else | ||
| stop | ||
| ! No need to test for every subcycling step | ||
| first_step_only: if (istep==nsteps) then |
There was a problem hiding this comment.
Is this IF-test really needed every single timestep - regardless of substeps code? We are just aborting with logic error - shouldn't that be done once and only once ever for a model run? How can the model use different arguments to subroutines at different timesteps? Just use a once and done ever instead???
There was a problem hiding this comment.
We could pass in a flag that tells the code whether this is the first time step or not, correct. This if test was added to avoid testing for every substep each time step. It should be something like
if (first_time_step .and. istep==nsteps) then
|
|
||
| #ifdef SION | ||
| !>\ingroup aathompson | ||
| subroutine readwrite_tables(filename, mode, mpicomm, mpirank, mpiroot, ierr) |
There was a problem hiding this comment.
Perhaps I am missing something, but is there no longer any opportunity to write out table files? Maybe it exists somewhere else, because this had something to do with SION, but the ability to read pre-existing or write new tables should probably still exist. The intent is that if someone wants to change an otherwise hard-wired parameter (e.g., the gamma distribution shape factor), then one or more tables would need to be changed.
There was a problem hiding this comment.
This only removes the logic to read and write tables in SIONlib format. You still have the original logic that reads and writes them as .dat files in the code. While the SIONlib format is far superior in I/O speed and scalability, it hasn't been picked up by any of the models, and there is a understandable push back for adding one more dependency to the model code.
|
Need to be careful here. The reset is used in two places. One to
indicate we're *'resetting'* the hourly max fields to 0 or -999 values,
but also when in the Thompson we want to compute the full radar
reflectivity (melti=T), which would be on the output time.
!Calculate hourly max 1-km agl and -10C reflectivity
if (lradar .and. (imp_physics == imp_physics_gfdl .or. &
imp_physics == imp_physics_thompson .or. &
imp_physics == imp_physics_fer_hires)) then
allocate(refd(im))
allocate(refd263k(im))
call max_fields(phil,refl_10cm,con_g,im,levs,refd,gt0,refd263k)
if (*reset*) then
do i=1,im
refdmax(i) = -35.
refdmax263k(i) = -35.
enddo
endif
do i=1,im
refdmax(i) = max(refdmax(i),refd(i))
refdmax263k(i) = max(refdmax263k(i),refd263k(i))
enddo
deallocate (refd)
deallocate (refd263k)
endif
!
if (*reset*) then
do i=1,im
spd10max(i) = -999.
u10max(i) = -999.
v10max(i) = -999.
t02max(i) = -999.
t02min(i) = 999.
rh02max(i) = -999.
rh02min(i) = 999.
pratemax(i) = 0.
…On 6/4/2021 10:03 AM, Dom Heinzeller wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In physics/mp_thompson.F90
<#666 (comment)>:
> @@ -566,7 +575,8 @@ subroutine mp_thompson_run(ncol, nlev, con_g, con_rd, &
ids=ids, ide=ide, jds=jds, jde=jde, kds=kds, kde=kde, &
ims=ims, ime=ime, jms=jms, jme=jme, kms=kms, kme=kme, &
its=its, ite=ite, jts=jts, jte=jte, kts=kts, kte=kte, &
- errmsg=errmsg, errflg=errflg, reset=reset)
+ reset=reset, istep=istep, nsteps=nsteps, &
This code is from @ericaligo-NOAA <https://github.com/ericaligo-NOAA>
and was merged some long time ago. Reset is a confusing word, I
totally agree. Maybe @ericaligo-NOAA
<https://github.com/ericaligo-NOAA> can suggest a better name.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#666 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALQ75MLIWJLXWNMSUSVP3J3TRDMMFANCNFSM45NPKF7Q>.
|
By design schemes do not have any "memory" of variables that depend on the domain decomposition. In other words, refdmax263k and other variables are coming from the host model. Why should Thompson MP contain code to reset such variables? In my opinion, this should be part of the other reset diag fields logic that sits in host-model specific interstitial code, not in the microphysics code itself. EDIT: I assume that resetting to |
|
I'm okay with the way it is now.
…On 6/4/2021 10:28 AM, Dom Heinzeller wrote:
Need to be careful here. The reset is used in two places. One to
indicate we're /'resetting'/ the hourly max fields to 0 or -999
values, but also when in the Thompson we want to compute the full
radar reflectivity (melti=T), which would be on the output time.
!Calculate hourly max 1-km agl and -10C reflectivity if
(lradar .and. (imp_physics == imp_physics_gfdl .or. &
imp_physics == imp_physics_thompson .or. &
imp_physics == imp_physics_fer_hires)) then
allocate(refd(im)) allocate(refd263k(im)) call
max_fields(phil,refl_10cm,con_g,im,levs,refd,gt0,refd263k)
if (/reset/) then do i=1,im
refdmax(i) = -35. refdmax263k(i) = -35.
enddo endif do i=1,im
refdmax(i) = max(refdmax(i),refd(i)) refdmax263k(i) =
max(refdmax263k(i),refd263k(i)) enddo deallocate (refd)
deallocate (refd263k) endif ! if (/reset/)
then do i=1,im spd10max(i) = -999.
u10max(i) = -999. v10max(i) = -999.
t02max(i) = -999. t02min(i) = 999.
rh02max(i) = -999. rh02min(i) = 999.
pratemax(i) = 0.
… <#>
By design schemes do not have any "memory" of variables that depend on
the domain decomposition. In other words, refdmax263k and other
variables are coming from the host model. Why should Thompson MP
contain code to reset such variables? In my opinion, this should be
part of the other reset diag fields logic that sits in host-model
specific interstitial code, not in the microphysics code itself.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#666 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALQ75MLO2CV4GJM3W75XXDTTRDPITANCNFSM45NPKF7Q>.
|
Let's hear from @gthompsnWRF . Maybe it's just a question of finding a better name that captures the two purposes of the variable. |
|
Feel free to make modifications especially if it will be clearer to the
user.
…On 6/4/2021 10:59 AM, Dom Heinzeller wrote:
I'm okay with the way it is now.
… <#>
Let's hear from @gthompsnWRF <https://github.com/gthompsnWRF> . Maybe
it's just a question of finding a better name that captures the two
purposes of the variable.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#666 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALQ75MIXX6USRZAGCOORQMLTRDS3ZANCNFSM45NPKF7Q>.
|
|
@gthompsnWRF I just pushed commit 490e67c that is supposed to fix the bug you pointed out and to address your other comments (except renaming |
Thanks, Eric. I will rename it to |
|
Yes, that sounds good. Thanks.
…On 6/4/2021 11:31 AM, gthompsnWRF wrote:
***@***.***WRF* commented on this pull request.
------------------------------------------------------------------------
In physics/mp_thompson.F90
<#666 (comment)>:
> @@ -566,7 +575,8 @@ subroutine mp_thompson_run(ncol, nlev, con_g, con_rd, &
ids=ids, ide=ide, jds=jds, jde=jde, kds=kds, kde=kde, &
ims=ims, ime=ime, jms=jms, jme=jme, kms=kms, kme=kme, &
its=its, ite=ite, jts=jts, jte=jte, kts=kts, kte=kte, &
- errmsg=errmsg, errflg=errflg, reset=reset)
+ reset=reset, istep=istep, nsteps=nsteps, &
Perhaps the simplest and more interpretable name is just: reset_dBZ.
How does that sound to everyone?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#666 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALQ75MLCAPLCJEZLXMKZQ43TRDWTLANCNFSM45NPKF7Q>.
|
|
You're welcome.
…On 6/4/2021 12:00 PM, Dom Heinzeller wrote:
Feel free to make modifications especially if it will be clearer
to the user.
… <#>
Thanks, Eric. I will rename it to |reset_dBZ| as Greg suggested.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#666 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALQ75MP34E3LPNROOSYIYWTTRD2BFANCNFSM45NPKF7Q>.
|
|
Commit 8ea6fc3 renames @gthompsnWRF Please check the PR again if anything else is missing or should be changed. Thanks for catching the |
|
@gthompsnWRF I will push the "re-request review" button. I believe/hope that I addressed all your comments. |
gthompsnWRF
left a comment
gthompsnWRF
left a comment
There was a problem hiding this comment.
Thanks for making the suggested changes/additions/etc. This now looks good to me.
Thanks, Greg. Note that this PR will be replaced with an identical PR (but different commit history) as part of a "Making code changes to the Unified Forecast System" tutorial. The new PR will refer to this PR for the discussion, review and approval. Just a heads up so that nobody gets confused. |
|
This PR was replaced with an identical PR #676 as part of the "UFS code commit process tutorial". |
* Add initialization for some local variables. * Change undefined value of aextc55 as 0 * Read soill to level 9 for RUC LSM * Correct tke reading and update upp revision. * Update upp revision to baa7751
5 participants
Description
This PR does the following:
Note that the ability to subcycle Thompson is completely optional and controlled by the suite definition file. There are now scientific changes necessary. If the number of subcycles/substep is set to one in the suite definition file, the logic and the results are identical.
Fixes #671
Testing
See ufs-community/ufs-weather-model#595
Dependencies
#666
NCAR/ccpp-framework#376
NOAA-EMC/ufsatm#316
ufs-community/ufs-weather-model#595